feat(audit): Add audit_request_id correlation field to all FGAC audit… by Taiwo435 · Pull Request #6343 · opensearch-project/security

Taiwo435 · 2026-07-28T22:50:48Z

Description

Add an audit_request_id field to all audit events so that multiple events generated by a single request (AUTHENTICATED → GRANTED_PRIVILEGES → COMPLIANCE_DOC_WRITE) can be correlated.

How it works

SecurityRestFilter reads the X-Request-Id header if present, otherwise generates a UUID
Sanitizes client-provided values (truncates to 128 chars, strips control characters)
Stored in ThreadContext as a transient for local events
AuditLogImpl.save() reads the transient first, falls back to X-Request-Id header (propagated by core across nodes)
All events from the same request share the same ID

Cross-node propagation

When a client sends X-Request-Id, OpenSearch core propagates it across nodes via Task.REQUEST_HEADERS. This means compliance events (COMPLIANCE_DOC_WRITE, COMPLIANCE_DOC_READ) on remote shard-hosting nodes also get the correlation ID.

Known limitation: Server-generated UUIDs (when no X-Request-Id header is sent) appear on user-initiated events (AUTHENTICATED, GRANTED_PRIVILEGES for the user action) but NOT on internally-spawned events COMPLIANCE_DOC_WRITE, auto-mapping INDEX_EVENT). This is because internal sub-actions run in a fresh ThreadContext where transients are cleared. When the client provides X-Request-Id, ALL events get the correlation ID because headers survive context changes.

References

OpenSearch core X-Request-Id support: Add Add X-Request-Id header to uniquely identify a search request OpenSearch#19798

Testing

Integration tests (AuditRequestIdCorrelationTest):
- Same request → all events share one ID
- Client-provided X-Request-Id header used verbatim
- UUID generated when no header present
- Different requests → different IDs
- Compliance doc write gets the request ID (when X-Request-Id sent)
Unit tests (AuditMessageTest): addRequestId() behavior
Existing test fix (WhoAmITests): exclude audit_request_id from structural equality comparison

Check List

New functionality includes testing
Commits are signed per the DCO using --signoff

github-actions · 2026-07-28T22:51:35Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 94d6b7e.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/filter/SecurityRestFilter.java	188	medium	Client-controlled X-Request-Id header is trusted and stored verbatim (after basic sanitization) into audit log correlation fields. An unauthenticated or malicious client can supply an arbitrary string (up to 128 chars, minus control chars) to appear in audit records, enabling audit trail spoofing — e.g., setting an ID matching a known legitimate request to create false correlations or obfuscate a malicious request's audit footprint. No authentication or signature validation is applied to the header value before it is accepted as authoritative.
src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java	95	low	The threadPool field visibility was widened from private to protected, expanding the accessible API surface for any subclass. While the immediate use in AuditLogImpl appears legitimate, this makes the thread context (and any secrets or transient values stored in it) accessible to future or third-party subclasses without explicit intent by the original authors.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | Low: 1

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-07-28T22:52:32Z

PR Reviewer Guide 🔍

(Review updated until commit `94d6b7e`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Sanitization Order The sanitization logic truncates to 128 chars before stripping control characters. If a client sends a value with control characters within the first 128 chars, the resulting sanitized ID could be shorter than expected, but more importantly, if control chars appear after position 128 they are truncated away (fine), but the truncation may cut a multi-byte sequence. Consider stripping control characters first, then truncating, so the length limit reflects the final stored value. // Sanitize: limit length and strip control characters to prevent log injection if (requestId.length() > 128) { requestId = requestId.substring(0, 128); } requestId = requestId.replaceAll("[\\p{Cntrl}]", ""); } Empty String Accepted `addRequestId` only rejects null, so an empty string is stored as the audit request ID (as also asserted by `testAddRequestIdEmptyStringIsStored`). Since `SecurityRestFilter` already checks `isEmpty()` and generates a UUID in that case, this is unlikely to trigger from REST, but a caller passing "" (e.g., via header propagation edge cases) would produce a useless correlation value. Consider rejecting empty/blank values here too. public void addRequestId(String requestId) { if (requestId != null) { auditInfo.put(REQUEST_ID, requestId); } }

github-actions · 2026-07-28T22:52:58Z

PR Code Suggestions ✨

Latest suggestions up to 94d6b7e
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Fallback to UUID after sanitization empties ID After sanitizing the client-provided `X-Request-Id`, the value may become empty (e.g., if the header contained only control characters, or the truncation/strip yields an empty string). In that case, fall back to a generated UUID so the audit correlation field is never empty and cannot be spoofed with a blank value. src/main/java/org/opensearch/security/filter/SecurityRestFilter.java [186-194] } else { // Sanitize: limit length and strip control characters to prevent log injection if (requestId.length() > 128) { requestId = requestId.substring(0, 128); } requestId = requestId.replaceAll("[\\p{Cntrl}]", ""); + if (requestId.isEmpty()) { + requestId = UUID.randomUUID().toString(); + } } threadContext.putTransient(ConfigConstants.SECURITY_AUDIT_REQUEST_ID, requestId); Suggestion importance[1-10]: 7 __ Why: Valid edge case: if the client-provided `X-Request-Id` contains only control characters or becomes empty after sanitization, the correlation ID would be empty. Falling back to a UUID ensures the field is always meaningful.	Medium
General	Avoid storing empty request IDs The current implementation stores empty strings, as documented by `testAddRequestIdEmptyStringIsStored`. Storing an empty audit correlation ID pollutes downstream logs and defeats the purpose of the field. Reject empty/blank values here so the field is either meaningful or absent. src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java [177-181] public void addRequestId(String requestId) { - if (requestId != null) { + if (requestId != null && !requestId.isEmpty()) { auditInfo.put(REQUEST_ID, requestId); } } Suggestion importance[1-10]: 5 __ Why: Reasonable defensive check to avoid empty correlation IDs, though the caller in `SecurityRestFilter` already sanitizes input. The suggestion would require updating the existing test `testAddRequestIdEmptyStringIsStored`.	Low

Previous suggestions

Suggestions up to commit d1ac125

Category	Suggestion	Impact
General	Fallback to reading request header directly `ThreadContext.getHeader(Task.X_REQUEST_ID)` returns the value OpenSearch itself sets from the request; however, if this filter runs before core sets it, the client-supplied `X-Request-Id` may not be reflected. Read the header directly from the `RestRequest` (case-insensitively) as a fallback so client-provided trace IDs are always honored, matching the integration test expectations. src/main/java/org/opensearch/security/filter/SecurityRestFilter.java [181-188] // Stash audit correlation ID: use X-Request-Id header if present, otherwise generate UUID if (threadContext.getTransient(ConfigConstants.SECURITY_AUDIT_REQUEST_ID) == null) { String requestId = threadContext.getHeader(Task.X_REQUEST_ID); if (requestId == null \|\| requestId.isEmpty()) { - requestId = UUID.randomUUID().toString(); + List<String> hdrs = request.getHeaders().get(Task.X_REQUEST_ID); + if (hdrs != null && !hdrs.isEmpty() && hdrs.get(0) != null && !hdrs.get(0).isEmpty()) { + requestId = hdrs.get(0); + } else { + requestId = UUID.randomUUID().toString(); + } } threadContext.putTransient(ConfigConstants.SECURITY_AUDIT_REQUEST_ID, requestId); } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a plausible concern about header propagation timing, but `ThreadContext.getHeader(Task.X_REQUEST_ID)` typically works for client-supplied headers. The improved code adds a reasonable fallback but the necessity is speculative.	Low
	Skip empty request id values Empty strings are stored as-is (as confirmed by `testAddRequestIdEmptyStringIsStored`), which pollutes audit logs with a useless `"audit_request_id":""` field. Skip empty/blank values so the field is only added when meaningful, and update the test accordingly. src/main/java/org/opensearch/security/auditlog/impl/AuditMessage.java [177-181] public void addRequestId(String requestId) { - if (requestId != null) { + if (requestId != null && !requestId.isEmpty()) { auditInfo.put(REQUEST_ID, requestId); } } Suggestion importance[1-10]: 5 __ Why: Skipping empty strings is a reasonable defensive improvement to avoid polluting audit logs with an empty field, though the impact is minor since callers control the input.	Low
	Make UUID pattern assertion less brittle `UUID.randomUUID().toString()` produces a version 4 UUID that includes uppercase-free hex but the OpenSearch REST client (or intermediaries) may also inject a `X-Opaque-Id`/`X-Request-Id` header that does not match this pattern, making this test flaky. Also, the test client may set its own `X-Request-Id`; assert on shape only after confirming no header was injected, or relax the pattern to `[0-9a-fA-F-]+`. src/integrationTest/java/org/opensearch/security/AuditRequestIdCorrelationTest.java [112-115] -// Should have a generated UUID (36 chars: 8-4-4-4-12) +// Should have a generated UUID (36 chars: 8-4-4-4-12) or client-injected id String requestId = (String) messages.get(0).getAsMap().get(AuditMessage.REQUEST_ID); assertThat(requestId, notNullValue()); -assertThat(requestId, matchesPattern("[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}")); +assertThat(requestId, matchesPattern("[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}")); Suggestion importance[1-10]: 3 __ Why: `UUID.randomUUID().toString()` always produces lowercase hex, so the original pattern is correct. The concern about injected headers is speculative and the relaxation offers minor value.	Low

… events Add a unique request correlation ID to every audit event so multiple events from the same REST request can be traced together. Uses the client-provided X-Request-Id header when available, otherwise generates a UUID per request as fallback. Implementation: - SecurityRestFilter: reads X-Request-Id header or generates UUID, stashes in ThreadContext as transient - AuditLogImpl.save(): reads from ThreadContext and stamps on every message before routing to sinks - AuditMessage: new REQUEST_ID field constant and addRequestId() setter Works across all FGAC event types: AUTHENTICATED, GRANTED_PRIVILEGES, MISSING_PRIVILEGES, INDEX_EVENT, COMPLIANCE_DOC_READ/WRITE, etc. Signed-off-by: Muzzamil Jolaade <muzzajol@amazon.com>

github-actions · 2026-07-29T00:53:24Z

Persistent review updated to latest commit 94d6b7e

codecov · 2026-07-29T01:21:07Z

Codecov Report

❌ Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.40%. Comparing base (283edfc) to head (94d6b7e).

Files with missing lines	Patch %	Lines
...opensearch/security/filter/SecurityRestFilter.java	50.00%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6343      +/-   ##
==========================================
+ Coverage   75.39%   75.40%   +0.01%     
==========================================
  Files         456      456              
  Lines       29924    29939      +15     
  Branches     4530     4534       +4     
==========================================
+ Hits        22560    22576      +16     
  Misses       5265     5265              
+ Partials     2099     2098       -1

Files with missing lines	Coverage Δ
...earch/security/auditlog/impl/AbstractAuditLog.java	`76.32% <ø> (ø)`
...pensearch/security/auditlog/impl/AuditLogImpl.java	`92.85% <100.00%> (+0.30%)`	⬆️
...pensearch/security/auditlog/impl/AuditMessage.java	`82.33% <100.00%> (+0.20%)`	⬆️
...g/opensearch/security/support/ConfigConstants.java	`95.65% <ø> (ø)`
...opensearch/security/filter/SecurityRestFilter.java	`86.98% <50.00%> (-1.84%)`	⬇️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Taiwo435 requested review from DarshitChanpura, Rishav9852Kumar, RyanL1997, cwperks, derek-ho, finnegancarroll, nibix, reta, shikharj05 and willyborankin as code owners July 28, 2026 22:50

Taiwo435 force-pushed the feat/audit-request-correlation branch from d1ac125 to 94d6b7e Compare July 29, 2026 00:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(audit): Add audit_request_id correlation field to all FGAC audit… - #6343

feat(audit): Add audit_request_id correlation field to all FGAC audit…#6343
Taiwo435 wants to merge 1 commit into
opensearch-project:mainfrom
Taiwo435:feat/audit-request-correlation

Taiwo435 commented Jul 28, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 28, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 28, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 28, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 29, 2026

Uh oh!

codecov Bot commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Taiwo435 commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How it works

Cross-node propagation

References

Testing

Check List

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Analyzer ❗

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 94d6b7e)

Uh oh!

github-actions Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented Jul 29, 2026

Uh oh!

codecov Bot commented Jul 29, 2026

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Taiwo435 commented Jul 28, 2026 •

edited

Loading

github-actions Bot commented Jul 28, 2026 •

edited

Loading

github-actions Bot commented Jul 28, 2026 •

edited

Loading

(Review updated until commit `94d6b7e`)

github-actions Bot commented Jul 28, 2026 •

edited

Loading